Skip to content
This repository was archived by the owner on Jan 15, 2025. It is now read-only.

[Lubuild] fix multiple bugs: case sensitive issue, default version issue and exception not throw out issue#576

Merged
feich-ms merged 30 commits intomasterfrom
feich/fixLubuildBug
Feb 27, 2020
Merged

[Lubuild] fix multiple bugs: case sensitive issue, default version issue and exception not throw out issue#576
feich-ms merged 30 commits intomasterfrom
feich/fixLubuildBug

Conversation

@feich-ms
Copy link
Contributor

@feich-ms feich-ms commented Feb 17, 2020 •
edited
Loading

Fix several bugs:

  1. As known to us, values of utterances/patterns are automatically converted to lower case in luis portal even though typed words are upper case. When comparing app json from lu file with the one from remote existing luis app, we should to ignore cases. To achieve this, I converted the objects to string, then to lower case so that the comparison works insensitively.
  2. Some properties in our luis definition don't align with the object pulled from luis portal. Fixed in this PR
  3. Default version 0.1 was included by previous PR when no version is specified which caused some bugs when updating version. In this PR, version from lu will count only if it's value is over than existing app. For example, if the app with the same app id in luis portal is version 0.1, and the version from the lu file is 0.3, 0.3 > 0.1, then 0.3 will count, otherwise, 0.1 will auto increase by 0.1 to 0.2
  4. Add test cases for above bug fixes
  5. Add test cases for luis:build refinements #548
  6. Fix bug Exceptions are not returned properly in luis:build #582

@feich-ms feich-ms changed the title [Lubuild] fix case sensitive issue when comparing luis applications json [Lubuild] fix case sensitive issue when comparing luis applications json to update Feb 17, 2020
vishwacsena
vishwacsena suggested changes Feb 17, 2020
View reviewed changes
Copy link
Contributor

@vishwacsena vishwacsena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add tests as well?

@feich-ms
Copy link
Contributor Author

feich-ms commented Feb 17, 2020

@vishwacsena, sure, will add tests soon.

@feich-ms
Copy link
Contributor Author

feich-ms commented Feb 18, 2020

Tests added @vishwacsena

vishwacsena
vishwacsena approved these changes Feb 18, 2020
View reviewed changes
Copy link
Contributor

@vishwacsena vishwacsena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@munozemilio it might make sense to expose compareApps as a public method on LUIS?

munozemilio
munozemilio reviewed Feb 19, 2020
View reviewed changes
munozemilio
munozemilio suggested changes Feb 19, 2020
View reviewed changes
Copy link
Contributor

@munozemilio munozemilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CIL

@feich-ms
Copy link
Contributor Author

feich-ms commented Feb 19, 2020

@munozemilio, comments resolved. Also added test cases for luConfig support of #548. Fixed bug of #582 as well.

@feich-ms feich-ms changed the title [Lubuild] fix case sensitive issue when comparing luis applications json to update [Lubuild] fix multiple bugs: case sensitive issue, default version issue and exception not throw out issue Feb 19, 2020
@feich-ms feich-ms mentioned this pull request Feb 19, 2020
Closed
munozemilio
munozemilio reviewed Feb 19, 2020
View reviewed changes
munozemilio
munozemilio reviewed Feb 19, 2020
View reviewed changes
munozemilio
munozemilio reviewed Feb 19, 2020
View reviewed changes
Copy link
Contributor

@munozemilio munozemilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments in line

@feich-ms
Copy link
Contributor Author

feich-ms commented Feb 20, 2020

Comments resolved. Thanks @munozemilio

@vishwacsena
Copy link
Contributor

vishwacsena commented Feb 21, 2020

@feich-ms I tried this E2E and pushed up two sets of deltas up to your branch. I'm not familiar with nock and some of the lubuild tests are failing. Can you take a look and address?

@vishwacsena
Copy link
Contributor

vishwacsena commented Feb 21, 2020

@munozemilio except for addressing tests, it would be good if you get a review as well. Thanks!

munozemilio
munozemilio reviewed Feb 21, 2020
View reviewed changes
munozemilio
munozemilio reviewed Feb 21, 2020
View reviewed changes
munozemilio
munozemilio reviewed Feb 21, 2020
View reviewed changes
munozemilio
munozemilio suggested changes Feb 21, 2020
View reviewed changes
Copy link
Contributor

@munozemilio munozemilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CIL

@feich-ms
Copy link
Contributor Author

feich-ms commented Feb 24, 2020 •
edited
Loading

@munozemilio, tests failures are fixed. Please help to approve. Thanks.

The merge is blocked by 'Request to change'. Could you approve if there are no other issues.

@feich-ms
Copy link
Contributor Author

feich-ms commented Feb 27, 2020 •
edited
Loading

@munozemilio, I added more modules in index.js and composerindex.js in the latest commit. My intention is to expose modules that composer client/broswer consumes in composerindex.js and other modules in index.js. Parser and SectionHandler is consumed by composer client and luBuild is consumed by composer server. Please let me know if this is not the reasonable way. Thanks.

munozemilio
munozemilio reviewed Feb 27, 2020
View reviewed changes
munozemilio
munozemilio approved these changes Feb 27, 2020
View reviewed changes
Copy link
Contributor

@munozemilio munozemilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with a comment

@feich-ms feich-ms merged commit 49f1871 into master Feb 27, 2020
@feich-ms feich-ms deleted the feich/fixLubuildBug branch February 27, 2020 05:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Danieladu Danieladu Awaiting requested review from Danieladu

@boydc2014 boydc2014 Awaiting requested review from boydc2014

2 more reviewers

@vishwacsena vishwacsena vishwacsena approved these changes

@munozemilio munozemilio munozemilio approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@feich-ms @vishwacsena @munozemilio